-
Notifications
You must be signed in to change notification settings - Fork 921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change babel plugin to use import map as source of truth #189
Conversation
0946ff0
to
eff3499
Compare
@@ -3,4 +3,4 @@ www/index.md | |||
www/dist | |||
/node_modules | |||
/web_modules | |||
__tests__/**/*/web_modules | |||
__tests__/integration/*/web_modules |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also ended up doing some cleanup of the test files. We had some test-generated files being added in git. Sorry for the spam!
dbe9a8c
to
b06e6a5
Compare
const explicitImportMap = path.join(process.cwd(), dir, explicitPath); | ||
return fs.readFileSync(explicitImportMap, {encoding: 'utf8'}); | ||
} | ||
const localImportMap = path.join(process.cwd(), dir, `import-map.local.json`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part will make a lot more sense when the next PR comes out. Feel free to skip for now.
885bbef
to
7d5eb94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven’t really spent a lot of time in the Babel plugin, but the general reasoning of this PR makes sense to me. +1 for deduplicating logic when it’s the same action and purpose.
7d5eb94
to
bcff8ca
Compare
bcff8ca
to
cc3852b
Compare
ac5cf73
to
4f2b939
Compare
There's a duplication of logic/responsibility between Snowpack and the babel plugin that's been bothering me for a while. Both are responsible for converting package names to
web_modules/
files:I've started a new project for Snowpack that will make it impossible to keep maintaining the logic in two places.
This PR makes it so that the babel plugin now just reads the info already contained inside the import map, and doesn't do any additional transformations on its own (besides
optionalExtensions
adding). This is now more rigid (will fail if it can't find the import map, can't find the package in the import map) but reading from a static map also means that everything is much less error prone, more deterministic, and probably a bit faster. You now know exactly where each import will be re-written to ahead of time.One interesting thing here is that we moved the 'addVersion' behavior from the plugin into Snowpack proper. All import-map users can now benefit from this logic as well, not just those using the plugin. For simplicity, I went with a file hash instead of the package version.